Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ccd0e29421
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| pub(crate) fn process_client_error(&mut self, id: RequestId, error: JSONRPCErrorError) { | ||
| tracing::error!("<- error: {:?}", JSONRPCError { id, error }); | ||
| } |
There was a problem hiding this comment.
Notify pending request callbacks on typed client errors
In the new typed in-process path (spawn_in_memory_typed), an AppServerClientMessage::Error is routed to process_client_error, but that method only logs and never calls outgoing.notify_client_error. When the server has an outstanding request waiting on a oneshot (created in OutgoingMessageSender::send_request_with_id), a client-side error response will leave the callback registered forever, so the awaiting task never resolves and the request map leaks until shutdown. This only occurs for typed clients that send the Error variant, but in that case it will hang any caller awaiting the response.
Useful? React with 👍 / 👎.
| params: v2::ThreadCompactParams, | ||
| response: v2::ThreadCompactResponse, | ||
| }, | ||
| ThreadShutdown => "thread/shutdown" { |
There was a problem hiding this comment.
ooc can we reuse turn/interrupt ?
|
Closing this pull request because it has had no updates for more than 14 days. If you plan to continue working on it, feel free to reopen or open a new PR. |
This is the piece of the original #10192 that adds the APIs we need to add to app server to support the TUI.
Perhaps I should wait until #10231 is in so I can update these with the new
#[experimental]annotation?